Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LADX: text shuffle exclusions #3919

Merged

Conversation

threeandthreee
Copy link
Contributor

@threeandthreee threeandthreee commented Sep 11, 2024

What is this fixing or adding?

Creates a list of exclusions for text shuffle.

zig wanted to exclude this when he initially implemented text shuffle but simply forgot and since then just didn't have time. This is preserving nobody's idea.

Having these options in text shuffle lessens the experience for all users as, for example, the Egg Path Textbox WILL be on another object but WILL NOT display the correct text (which path it is) which leads to users being confused and frustrated because they believe the shuffled textbox is correct. They also shuffle the path when talking to the book for the first time WITHOUT being able to EVER find out the right path without having the meta knowledge of knowing all four paths.

Excludes:

  • Owl statues (hints)
  • Library books (hints, egg path)
  • Signpost by egg indicating goal
  • Signs in the signpost maze
  • All text containing rupee prices

How was this tested?

Generated solo seeds, verified intended text was unshuffled.

Exclude owl statues, library books, goal sign, signpost maze, and various rupee prices from text shuffle
@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Sep 11, 2024
@Exempt-Medic Exempt-Medic added the is: enhancement Issues requesting new features or pull requests implementing new features. label Sep 11, 2024
@Exempt-Medic
Copy link
Member

Again, I feel like this would make sense as a user-facing option. It could by default do this, but I think it would make sense to at least have the option be available

@palex00
Copy link
Contributor

palex00 commented Sep 13, 2024

Again, I feel like this would make sense as a user-facing option. It could by default do this, but I think it would make sense to at least have the option be available

I'm a big believer in YAML options but this is like asking to have a YAML option to keep erronous implementations in.

zig wanted to exclude this when he initially implemented text shuffle but simply forgot and since then just didn't have time. This is preserving nobody's idea.

Having these options in text shuffle lessens the experience for all users as, for example, the Egg Path Textbox WILL be on another object but WILL NOT display the correct text (which path it is) which leads to users being confused and frustrated because they believe the shuffled textbox is correct. They also shuffle the path when talking to the book for the first time WITHOUT being able to EVER find out the right path without having the meta knowledge of knowing all four paths.

@threeandthreee Just to make sure: Will this also prevent those text boxes to be in the pool? So will the Egg path be anywhere else if the original egg path is not text shuffled?

@threeandthreee
Copy link
Contributor Author

@threeandthreee Just to make sure: Will this also prevent those text boxes to be in the pool? So will the Egg path be anywhere else if the original egg path is not text shuffled?

Each path text is excluded, yeah. Should only show up in the book and should be the correct one as its completely left alone, though I didn't directly test that.

@Exempt-Medic
Copy link
Member

Again, I feel like this would make sense as a user-facing option. It could by default do this, but I think it would make sense to at least have the option be available

I'm a big believer in YAML options but this is like asking to have a YAML option to keep erronous implementations in.

zig wanted to exclude this when he initially implemented text shuffle but simply forgot and since then just didn't have time. This is preserving nobody's idea.

Having these options in text shuffle lessens the experience for all users as, for example, the Egg Path Textbox WILL be on another object but WILL NOT display the correct text (which path it is) which leads to users being confused and frustrated because they believe the shuffled textbox is correct. They also shuffle the path when talking to the book for the first time WITHOUT being able to EVER find out the right path without having the meta knowledge of knowing all four paths.

@threeandthreee Just to make sure: Will this also prevent those text boxes to be in the pool? So will the Egg path be anywhere else if the original egg path is not text shuffled?

Then the full extent of why changes are being made should be included in the PR description. None of what you just said was in any way apparent

@palex00
Copy link
Contributor

palex00 commented Sep 14, 2024

Then the full extent of why changes are being made should be included in the PR description. None of what you just said was in any way apparent
@threeandthreee Can you include my excerpt about why this change is needed in the start post?

Copy link

@SushiKishi SushiKishi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The feature is completely unusable in my opinion without this addition. There's randomizing text, and there's unexpected and usual gameplay in a randomizer, and sometimes game can't completely support providing all of the information needed for Archipelago and you gotta keep an eye on your text client. That's all fine.

Without this change, though, this 'feature' simply straight up lies to the player and obfuscates critical information such as "how do I finish the game." The changes proposed complete development of a feature that was only ever partially implemented.

Copy link
Contributor

@palex00 palex00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR was tested as part of a Beta apworld that was used in three dedicated Test Asyncs, as well as dozen of normal Syncs. I have also personally run 200 test generations which succeeded.

The new feature has worked as expected and other stuff does not seem impacted.

I explained why we need this PR especially in a previous comment: #3919 (comment)

@NewSoupVi NewSoupVi merged commit 85a0d59 into ArchipelagoMW:main Dec 5, 2024
16 checks passed
@threeandthreee threeandthreee deleted the ladx/text-shuffle-exclusions branch December 10, 2024 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: enhancement Issues requesting new features or pull requests implementing new features. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants